Skip to content

Conversation

yaacovCR
Copy link
Contributor

depends on #4026

removed filtering again

Copy link

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

Copy link

netlify bot commented Mar 21, 2024

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit a043386
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/660fde76ee34ba0008b5bce7
😎 Deploy Preview https://deploy-preview-4032--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@yaacovCR

This comment has been minimized.

Copy link

@github-actions publish-pr-on-npm

@yaacovCR The latest changes of this PR are available on NPM as
graphql@17.0.0-alpha.3.canary.pr.4032.4fb41fe3e1f2b4b27437138d6d7d4763c1992e7a
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-4032

@yaacovCR
Copy link
Contributor Author

Looks like this PR is about 5-10% slower than #4026 using the supermassive benchmark from #4008.

As usual, the results are pretty noisy, what I am most interested in is the run for already parsed queries, and the noise seems low enough to get a comparison.

So @robrichard, the question is whether we can optimize this further and make up that ground and whether the 5-10% differential is worth the divergence from the spec edits.

This PR:

yaaco@Yaacov-15 MINGW64 ~/Documents/GitHub/graphitation/packages/supermassive (main)
$ yarn run benchmark
yarn run v1.22.19
$ node --require ts-node/register ./src/benchmarks/index.ts
Query parsing
graphql-js x 160,596 ops/sec ±3.96% (74 runs sampled)
Query Running
graphql-js - string queries x 44,479 ops/sec ±1.92% (73 runs sampled)
graphql-js - parsed queries x 71,218 ops/sec ±2.52% (73 runs sampled)
Done in 20.70s.

yaaco@Yaacov-15 MINGW64 ~/Documents/GitHub/graphitation/packages/supermassive (main)
$ yarn run benchmark
yarn run v1.22.19
$ node --require ts-node/register ./src/benchmarks/index.ts
Query parsing
graphql-js x 153,906 ops/sec ±4.54% (69 runs sampled)
Query Running
graphql-js - string queries x 45,403 ops/sec ±1.98% (78 runs sampled)
graphql-js - parsed queries x 74,677 ops/sec ±2.39% (68 runs sampled)
Done in 20.63s.

yaaco@Yaacov-15 MINGW64 ~/Documents/GitHub/graphitation/packages/supermassive (main)
$ yarn run benchmark
yarn run v1.22.19
$ node --require ts-node/register ./src/benchmarks/index.ts
Query parsing
graphql-js x 162,527 ops/sec ±4.45% (73 runs sampled)
Query Running
graphql-js - string queries x 45,545 ops/sec ±1.88% (74 runs sampled)
graphql-js - parsed queries x 72,569 ops/sec ±2.06% (71 runs sampled)
Done in 21.02s.

yaaco@Yaacov-15 MINGW64 ~/Documents/GitHub/graphitation/packages/supermassive (main)
$ yarn run benchmark
yarn run v1.22.19
$ node --require ts-node/register ./src/benchmarks/index.ts
Query parsing
graphql-js x 151,312 ops/sec ±4.19% (68 runs sampled)
Query Running
graphql-js - string queries x 44,999 ops/sec ±1.88% (72 runs sampled)
graphql-js - parsed queries x 73,783 ops/sec ±2.13% (72 runs sampled)
Done in 20.55s.

With #4026:

yaaco@Yaacov-15 MINGW64 ~/Documents/GitHub/graphitation/packages/supermassive (main)
$ yarn run benchmark
yarn run v1.22.19
$ node --require ts-node/register ./src/benchmarks/index.ts
Query parsing
graphql-js x 151,039 ops/sec ±4.55% (69 runs sampled)
Query Running
graphql-js - string queries x 43,278 ops/sec ±5.44% (69 runs sampled)
graphql-js - parsed queries x 77,934 ops/sec ±1.89% (68 runs sampled)
Done in 20.52s.

yaaco@Yaacov-15 MINGW64 ~/Documents/GitHub/graphitation/packages/supermassive (main)
$ yarn run benchmark
yarn run v1.22.19
$ node --require ts-node/register ./src/benchmarks/index.ts
Query parsing
graphql-js x 159,241 ops/sec ±3.79% (73 runs sampled)
Query Running
graphql-js - string queries x 45,811 ops/sec ±2.12% (71 runs sampled)
graphql-js - parsed queries x 77,033 ops/sec ±2.00% (70 runs sampled)
Done in 20.60s.

yaaco@Yaacov-15 MINGW64 ~/Documents/GitHub/graphitation/packages/supermassive (main)
$ yarn run benchmark
yarn run v1.22.19
$ node --require ts-node/register ./src/benchmarks/index.ts
Query parsing
graphql-js x 157,320 ops/sec ±3.99% (72 runs sampled)
Query Running
graphql-js - string queries x 43,644 ops/sec ±4.40% (70 runs sampled)
graphql-js - parsed queries x 77,631 ops/sec ±1.81% (71 runs sampled)
Done in 20.95s.

yaaco@Yaacov-15 MINGW64 ~/Documents/GitHub/graphitation/packages/supermassive (main)
$ yarn run benchmark
yarn run v1.22.19
$ node --require ts-node/register ./src/benchmarks/index.ts
Query parsing
graphql-js x 158,876 ops/sec ±3.38% (72 runs sampled)
Query Running
graphql-js - string queries x 40,389 ops/sec ±8.26% (63 runs sampled)
graphql-js - parsed queries x 78,750 ops/sec ±2.36% (68 runs sampled)
Done in 20.58s.

@yaacovCR yaacovCR requested a review from robrichard March 21, 2024 11:23
@yaacovCR yaacovCR added the PR: polish 💅 PR doesn't change public API or any observed behaviour label Mar 21, 2024
@yaacovCR yaacovCR changed the title incremental: remove filtering again incremental: remove filtering from rewrite Mar 21, 2024
@yaacovCR

This comment has been minimized.

Copy link

@github-actions publish-pr-on-npm

@yaacovCR The latest changes of this PR are available on NPM as
graphql@17.0.0-alpha.3.canary.pr.4032.8bcdcea90e0a24432a78270866c27e0db6a2ae4d
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-4032

@yaacovCR yaacovCR force-pushed the remove-filtering-again branch 3 times, most recently from 5fe0b2f to f2ec6a0 Compare March 28, 2024 21:40
@yaacovCR
Copy link
Contributor Author

image

take-two = #4026
remove-filtering-again = this PR

@yaacovCR yaacovCR force-pushed the remove-filtering-again branch 2 times, most recently from bff8561 to ba7770a Compare April 5, 2024 08:29
@yaacovCR yaacovCR force-pushed the remove-filtering-again branch from ba7770a to a043386 Compare April 5, 2024 11:20
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Apr 5, 2024

I've actually gotten some better results with this, so I am going to fold this into #4026. We can always re-introduce filtering again if need be, but it would be nice if the implementation could match the spec.

@yaacovCR yaacovCR closed this Apr 5, 2024
@yaacovCR yaacovCR deleted the remove-filtering-again branch September 5, 2024 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: polish 💅 PR doesn't change public API or any observed behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant